fix: add mobile hamburger menu to header#357
fix: add mobile hamburger menu to header#357mohanakatari119-bit wants to merge 4 commits intoopen-telemetry:mainfrom
Conversation
Signed-off-by: mohanakatari119-bit <mohana.katari119@gmail.com>
✅ Deploy Preview for otel-ecosystem-explorer ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR adds a mobile navigation fallback to the shared Header so the primary site links remain reachable below the md breakpoint. It fits into the app shell by extending the persistent top-level header component and updating its tests.
Changes:
- Added a mobile hamburger toggle to
Header, with conditional mobile nav rendering and ARIA attributes. - Kept the existing desktop navigation and added labels for navigation landmarks.
- Expanded
Headertests to cover the new toggle button, open/close state, and menu-closing behavior on link click.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ecosystem-explorer/src/components/layout/header.tsx |
Adds menu state, mobile toggle button, and conditional mobile nav to the fixed app header. |
ecosystem-explorer/src/components/layout/header.test.tsx |
Adds interaction tests for the new mobile menu behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jaydeluca
left a comment
jaydeluca
left a comment
There was a problem hiding this comment.
great contribution @mohanakatari119-bit thank you for addressing this!
The two comments from copilot are valid, i confirmed the menu stays open when you navigate without pressing a button in the opened menu, so that would be good to address. The second comment about having a unified nav-item definition would be a nice improvement too if you wouldn't mind looking into that.
Thanks again!
thanks :) |
mohanakatari119-bit
force-pushed
the
fix/mobile-nav-hamburger
branch
from
49dc1a1 to
4a7b1a5
Compare
|
@jaydeluca hey i just fixed the suggestions, can u review it : ) |
vitorvasc
left a comment
vitorvasc
left a comment
There was a problem hiding this comment.
Looks great! Thanks for the contribution 🙌🏼
Two minor tweaks that could improve the experience:
- The menu currently stays open even when clicking outside of it
- We could add an opaque backdrop behind the menu t keep the focus on the navigation while it's open. This would also make it easier to handle the "click outside" behavior from the previous item
Let me know what you think :)
|
hey @vitorvasc, both make sense! the backdrop especially would make the close-on-outside-click much cleaner to handle. i'll work on both together since they pair well. will push an update soon |
The header navigation was only visible on
mdand above breakpoints (hidden ... md:flex). On mobile there was no fallback, so the nav links were completely inaccessible to anyone on a phone or narrow viewport.This adds a hamburger toggle button that is visible only below the
mdbreakpoint. Clicking it slides open a vertical nav panel with the same links. The icon swaps betweenMenuandX(from lucide-react, already a dependency). The panel closes automatically when a link is tapped so the user lands on the new page without having to dismiss the menu manually.Both the toggle button and the nav panels carry appropriate
aria-label,aria-expanded, andaria-controlsattributes. The desktop<nav>now also hasaria-label="Main"which was missing.Three new test cases cover: hamburger button presence, open/close toggle state, and auto-close on link click.
demo